Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow a specified address to disable/enable the Exchange #1467

Merged
merged 16 commits into from
Nov 1, 2019

Conversation

m-chrzan
Copy link
Contributor

@m-chrzan m-chrzan commented Oct 24, 2019

Description

For purposes of Baklava, we might need to periodically turn the Exchange off. This PR adds freeze and unfreeze functions to the Exchange, allowing an address specified at migration time (and modifiable via setFreezer) to disable the exchange function.

Tested

Unit tests.

Other changes

This PR adds a new helper contract, Freezable, that specifies a freezer address and provides modifiers to disable certain contract functions if the contract becomes "frozen".

This is in anticipation of a similar change needed in LockedGold.

Related issues

Backwards compatibility

Not backwards compatible, changes Exchange storage layout.

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #1467 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #1467    +/-   ##
========================================
  Coverage   73.61%   73.61%            
========================================
  Files         277      277            
  Lines        7443     7443            
  Branches      956      662   -294     
========================================
  Hits         5479     5479            
  Misses       1852     1852            
  Partials      112      112
Flag Coverage Δ
#mobile 73.61% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8d97af...2308c48. Read the comment docs.

Copy link
Contributor

@asaj asaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍


// onlyWhenNotFrozen functions can only be called when `frozen` is false, otherwise they are
// noops.
modifier onlyWhenNotFrozen() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? I would consider cutting this function and renaming onlyWhenNotFrozenOrThrow to onlyWhenNotFrozen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have cleared this up - this is already looking forward to the next change, where distributeEpochPayments will need to be frozen. By not reverting when that function is frozen, I think we'll be able to contain this temporary change for Baklava to smart contracts.

I could see an argument for reverting being the default semantic, happy to rename this one to onlyWhenNotFrozenNoThrow.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we don't want to revert when we freeze epoch rewards for TGCSO? Reverting would prevent any proof-of-stake action from happening in Finalize, e.g. validator score changes, payments, etc.

@asaj asaj assigned m-chrzan and unassigned asaj Oct 31, 2019
@m-chrzan m-chrzan assigned asaj and unassigned m-chrzan Nov 1, 2019
@asaj asaj assigned m-chrzan and unassigned asaj Nov 1, 2019
@asaj
Copy link
Contributor

asaj commented Nov 1, 2019

Did you mean to re-assign to me?

edit: Whoops, missed your comment

@m-chrzan m-chrzan added the automerge Have PR merge automatically when checks pass label Nov 1, 2019
@celo-ci-bot-user celo-ci-bot-user merged commit 82aac04 into master Nov 1, 2019
@celo-ci-bot-user celo-ci-bot-user deleted the m-chrzan/toggle-exchange branch November 1, 2019 23:56
aaronmgdr added a commit that referenced this pull request Nov 2, 2019
* master: (62 commits)
  Fix e2e on CI (#1537)
  Allow a specified address to disable/enable the Exchange  (#1467)
  Avoid re-encrypting key files with yarn keys:encrypt command (#1560)
  Support protocol hotfixing (#613)
  Point e2e tests back (#1562)
  Refactor to Accounts.sol (#1392)
  Add selectIssuers Transaction (#1327)
  [Wallet] Get React Native Hot Reloading Working (#1551)
  Unify to prefix messages for signing (#1473)
  [Wallet] Improve error handling around account creation and keystore ops (#1497)
  Add CI test for checking licenses and misc package.json cleanup (#1550)
  [Wallet] Implement SMS invite on iOS (#1541)
  CI: brings back to master (#1554)
  Validators: uses Ethereum address for proof of possession (#1494)
  Validate Attestation Requests (#1468)
  Rename hosted node references to forno (#1511)
  Bump rubyzip from 1.2.3 to 1.3.0 in /packages/mobile (#1508)
  Added txpool family to geth apis. Sorted geth cmd options (#1462)
  [Wallet] Fix yarn dev command for running android (#1534)
  [Wallet] iOS info plist changes and version bump (#1539)
  ...

# Conflicts:
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Dec 5, 2019
* master: (73 commits)
  Fix Ethstats Image reference (#1577)
  EU Cookies Behavior Change (#1447)
  [verifier] Upgrade to RN 61 (#1572)
  [Wallet] Update link styles and Implement VerificationEducationScreen (#1565)
  [wallet] Added native phone picker (#1310)
  [Wallet] Set up new verification screen skeletons (#1563)
  Bump e2e test migrate numbers where needed (#1567)
  [Wallet] Create new carousel component (#1555)
  [Wallet] Protect Backup Key and Safeguards with PIN (#1556)
  Increase ganache gas limit (#1569)
  Re-work locked gold requirements for validators and groups (#1474)
  Fix e2e on CI (#1537)
  Allow a specified address to disable/enable the Exchange  (#1467)
  Avoid re-encrypting key files with yarn keys:encrypt command (#1560)
  Support protocol hotfixing (#613)
  Point e2e tests back (#1562)
  Refactor to Accounts.sol (#1392)
  Add selectIssuers Transaction (#1327)
  [Wallet] Get React Native Hot Reloading Working (#1551)
  Unify to prefix messages for signing (#1473)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants